refactor(run): collapse dual permission handling paths (F10)#13
refactor(run): collapse dual permission handling paths (F10)#13tesdal wants to merge 1 commit intophase-ab-basefrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors opencode run’s permission.asked SSE handling to route through a single dispatcher, eliminating the prior dual-path structure that could lead to double-responders (audit finding F10), and adds targeted unit tests to pin the invariant.
Changes:
- Added
replyPermissionAttachModeanddispatchPermissionAskedhelpers and delegated SSE-loop permission handling to them. - Introduced a structural
PermissionReplyClienttype for the SDK permission reply surface. - Added
run-attach-permissionunit tests covering helper behavior and the F10 “single responder” contract.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/opencode/src/cli/cmd/run.ts | Extracts attach/non-attach permission reply logic into exported helper(s) and updates the SSE loop to delegate. |
| packages/opencode/test/cli/run-attach-permission.test.ts | Adds unit tests validating attach/non-attach behavior and preventing double SDK replies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Dispatch a `permission.asked` SSE event to either the no-op-with-log path | ||
| * (non-attach: `runEventsHandle` is set, server-side RunEvents owns the reply) | ||
| * or the attach-mode reply path (`runEventsHandle` is null, this client must | ||
| * reply via SDK). |
There was a problem hiding this comment.
Doc says non-attach mode is where “server-side RunEvents owns the reply”, but in this file non-attach mode runs RunEvents.make in the same CLI process. Consider changing wording to “in-process/local RunEvents” to avoid confusion with attach mode’s remote server context.
There was a problem hiding this comment.
Good catch — fixed in 49d67d9. Replaced 'server-side RunEvents' with 'in-process RunEvents' / 'in-process alongside prompt.loop' across both helpers' doc blocks and the call-site inline comment. Kept the failure-mode hypothetical at line ~286 ('e.g. attach mode also gets a server-side RunEvents') because there it correctly refers to a literal future remote-side RunEvents on the attached server.
| * Behavior matrix (mirrors RunEvents semantics): | ||
| * - skipPermissions=true → reply "once" (silent; symmetric with auto-approve flow) | ||
| * - skipPermissions=false, jsonMode=false → log + reply "reject" | ||
| * - skipPermissions=false, jsonMode=true → reply "reject" without UI (caller emits JSON) |
There was a problem hiding this comment.
The behavior matrix says jsonMode=true rejects “without UI (caller emits JSON)”, but in attach mode there is no RunEvents instance to emit the auto-reject/auto-approve JSON events, and run.ts doesn’t emit JSON for permission.asked either. This makes attach+jsonMode silently auto-reject with no JSON signal; either update the doc to say it’s silent, or add a JSON emission path for permission auto-replies in attach mode to match RunEvents’ jsonMode behavior.
| * Behavior matrix (mirrors RunEvents semantics): | |
| * - skipPermissions=true → reply "once" (silent; symmetric with auto-approve flow) | |
| * - skipPermissions=false, jsonMode=false → log + reply "reject" | |
| * - skipPermissions=false, jsonMode=true → reply "reject" without UI (caller emits JSON) | |
| * Behavior matrix for attach mode: | |
| * - skipPermissions=true → reply "once" (silent; symmetric with auto-approve flow) | |
| * - skipPermissions=false, jsonMode=false → log + reply "reject" | |
| * - skipPermissions=false, jsonMode=true → reply "reject" without UI or JSON emission |
There was a problem hiding this comment.
Right — attach + jsonMode is silent (no UI, no JSON event). Updated the behavior matrix in 49d67d9 to reflect this honestly: 'reply reject without UI or JSON emission'. Added a non-blocking followup note explaining the parity gap and why fixing it is out of scope for F10 (would require either an attach-side JSON emitter or moving JSON into a sink shared with RunEvents — that latter belongs to F11).
run.ts had two permission responders — the SSE-loop branch (lines
559-574) and the RunEvents bus subscriber — gated only by 'if
(runEventsHandle)'. The gate is correct today but the structure
invites a future race where both fire.
Investigation showed runEventsHandle is null exactly when args.attach
is set (run.ts:648), and the remote opencode server does not
currently spin up its own RunEvents handler. So the SSE-loop branch
has real semantics in attach mode: it is the only auto-responder for
permission asks visible to the local user.
Choice: option 2 from the F10 plan — keep the attach-mode reply, but
extract it into a clearly-named exported helper
(replyPermissionAttachMode) with a coupling-note doc block that
documents the invariant (and how a future server-side RunEvents would
make this helper a redundant double-responder).
The SSE-loop block is now structurally:
if (runEventsHandle) { /* log only, then continue */ }
await replyPermissionAttachMode({...})
eliminating the dual-path ambiguity while preserving today's behavior.
Tests: 4 new unit tests in test/cli/run-attach-permission.test.ts
covering each cell of the (skipPermissions × jsonMode) matrix and a
no-double-reply pin. 18/18 pass across run-events + new file.
Addresses audit finding F10 (Opus diamond review, 2026-04-22).
tesdal
force-pushed
the
local/audit-f10-dual-permission-path
branch
from
b189388 to
49d67d9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Review-only PR — Copilot R2 clean (no new comments). Closing per per-finding workflow; merging local/audit-f10-dual-permission-path into local/integration-v2 with --no-ff next. |
Audit finding F10 (Opus diamond review, 2026-04-22). Diamond: codex-5.3 R1 REQUEST_CHANGES (e2e gap) → R2 APPROVE_WITH_NITS; opus R1 APPROVE_WITH_NITS → R2 APPROVE_WITH_NITS. Copilot review: PR #13 (review-only, closed). 2 rounds: R1: 2 substantive (1) 'server-side RunEvents' wording was wrong (RunEvents runs in-process), 2) attach+jsonMode silently auto-rejects without JSON parity vs non-attach. Both fixed by doc updates + non-blocking followup note (parity belongs to F11 sink-extraction). R2: clean (no new comments). Refactor extracts attach-mode reply into replyPermissionAttachMode and the dual-path decision into dispatchPermissionAsked. Both exported for unit-test access (no public consumers). 9 unit tests pin the F10 invariant: at most one sdk.permission.reply per permission.asked event. Net delta: +353 / -30 LOC (mostly tests + doc blocks).
Summary
Audit finding F10 (Opus diamond review, 2026-04-22):
run.tshad two permission responders gated only byif (runEventsHandle). Investigation showed the gate is correct today (runEventsHandle === null⇔args.attach), but the structure invited a future race.Changes
replyPermissionAttachModewith a coupling-note doc block.dispatchPermissionAskedso the SSE-loop just delegates.Pick<OpencodeClient, "permission">to a structuralPermissionReplyClient.Tests
9 unit tests in
test/cli/run-attach-permission.test.ts:replyPermissionAttachMode's (skipPermissions × jsonMode) behavior.sdk.permission.reply(3 flag combinations)Diamond review
Verification
bun typecheckcleanbun test test/cli/run-attach-permission.test.ts test/cli/run-events.test.ts→ 23/23 passNet delta: +344 / -30 LOC (mostly tests + doc blocks).